-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add jobs #314
feat: add jobs #314
Conversation
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generally looks good — but approving with the caveat that I don't feel like I fully grok some of the mechanisms at play here. Would be great to walk through the PR together so I can ask questions.
@toph-allen and I met over Zoom to discuss his questions and talk about the code in more depth. Please let me know if anyone else would like to do the same. Happy to do so. |
Adds job support.
The mixin pattern is continued, and a 'jobs' property is added to
ContentItem.
A new abstraction around resources called
Active
is introduced. This abstraction begins a transition to a more idiomatic approach to interacting with collections. Similar to how printing aResource
shows adict
, theActiveSequence
shows a familiarList[dict]
representation. TheActiveFinderMethods
supports thefind
andfind_by
methods. This is inspired by this Ruby on Rails module https://api.rubyonrails.org/classes/ActiveRecord/FinderMethods.html. Ideally, this separation of concerns provides a straightforward way to compose bindings in the future and reduces the amount of duplication across implementations.Additional background may be found here https://github.com/tdstein/active
Related to #310
Resolves #306